-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Sanitize ini-options default handling #11282 #11594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, left some suggestions please take a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SharadNair7!
FYI I did some more tweaks in the code. 👍
Hello Zac (@Zac-HD), May I request you to please review this PR as it has been already reviewed and approved by Bruno (@nicoddemus), so that this can be merged to the code base. Regards |
Please don't ping people who haven't opted into that - we can subscribe to issues or whole repos if we want to! I'm on a work trip at the moment and my OSS time is going to projects where I'm the sole maintainer, so I won't be able to review this for a while - but I trust Bruno's judgement and review. |
Sorry for the trouble Zac, I totally trust Bruno's review too. It was just that I felt 2 reviews were needed for merging and my past interaction with you had been helpful hence included you. Sorry once again for the inconvenience including this message.
Regards
Sharad
…________________________________
From: Zac Hatfield-Dodds ***@***.***>
Sent: Thursday, November 9, 2023 10:19:28 AM
To: pytest-dev/pytest ***@***.***>
Cc: sharad.nair progenous.com ***@***.***>; Mention ***@***.***>
Subject: Re: [pytest-dev/pytest] Sanitize ini-options default handling #11282 (PR #11594)
Please don't ping people who haven't opted into that - we can subscribe to issues or whole repos if we want to! I'm on a work trip at the moment and my OSS time is going to projects where I'm the sole maintainer, so I won't be able to review this for a while - but I trust Bruno's judgement and review.
—
Reply to this email directly, view it on GitHub<#11594 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BAFOT5GQZCA7HDWCHCTQN3LYDROFRAVCNFSM6AAAAAA7BE2JVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTGE2TQNJZGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I will merge this in the next few days to give time in case others want to review it. 👍 |
Thank you Bruno.
Regards
|
Thanks @SharadNair7 for another contribution, greatly appreciated! |
Thank you for the kind appreciation, Bruno. Means a lot.
Regards
Sharad
|
Overview
This PR fixes #11282. As per the issue, if an option is created with default value as None and a value for the option is not set in the ini file, then trying to get the option value in code with config.getini() returns an empty list [] instead of None as expected.
Analysis
On investigating it was found that the logic to return a default value for a configuration option is as follows:
if a default value is set during the creation of the option, then the set default value is returned
if a default value is NOT set during the option creation but the type for the option is provided, then return an empty list
if neither a default value nor type is provided while creating the option, then assume its a string type and return an empty string
The above logic did not seem right. Also, the logic above did not honor None as an explicit default value to return.
Solution
The solution provided in this PR will change the behavior in the following way:
if a default value is NOT set during the option creation but the type for the option is provided, then a type specific default will be returned. That is as follows:
"paths", "pathlist", "args", "linelist" types will have a default empty list [] as these are all lists
"bool" will have False as default
"string" will have empty string "" as default
logically there can't be any other type as the code checks for valid types before hand. However, just as a fail safe for future, any other types will have a default of None
if a default value is set explicitly as None during the option creation, then None will be returned as the default value irrespective of the type
if neither a default value nor type is provided while creating the option, assume its a string type and return an empty string. (This assumption stays as it is from the existing code)
Checklist